Fix ODB: DEF_file in 3dbx (Issue #10077) by openroad-ci · Pull Request #10096 · The-OpenROAD-Project/OpenROAD

openroad-ci · 2026-04-09T19:43:25Z

fix issue #10077

github-actions · 2026-04-09T19:47:48Z

clang-tidy review says "All clean, LGTM! 👍"

gemini-code-assist

Code Review

This pull request introduces logic to read DEF files for chiplets and chip instances, using a boolean property to track whether the file has already been processed. The review highlights a potential for redundant DEF reads in createChiplet due to a missing check, suggests refactoring the duplicated DEF reading logic into a shared helper function, and recommends using std::vector::assign for more efficient population of the search_libs vector.

gemini-code-assist · 2026-04-09T19:49:42Z

                        chiplet.external.def_file.c_str(),
                        chip,
                        /*issue_callback*/ false);
+    odb::dbBoolProperty::create(chip, "def_file_read", true);


While setting the def_file_read property here is correct, the corresponding check to see if it's already set (similar to the one added in createChipInst at line 683) is missing at the beginning of the DEF reading block in this function (around line 456). This could lead to redundant DEF reads if createChiplet is called multiple times for the same master chip.

gemini-code-assist · 2026-04-09T19:49:42Z

+  if (!chip_inst.external.def_file.empty()) {
+    if (odb::dbProperty::find(chip, "def_file_read") == nullptr) {
+      if (chip->getBlock() != nullptr) {
+        odb::dbBlock::destroy(chip->getBlock());
+      }
+      odb::defin def_reader(db_, logger_, odb::defin::DEFAULT);
+      std::vector<odb::dbLib*> search_libs;
+      for (odb::dbLib* lib : db_->getLibs()) {
+        search_libs.push_back(lib);
+      }
+      def_reader.readChip(
+          search_libs, chip_inst.external.def_file.c_str(), chip, false);
+      odb::dbBoolProperty::create(chip, "def_file_read", true);
+    }
+  }


The logic for reading a DEF file for a chiplet is duplicated between createChiplet and createChipInst. This logic (checking for the property, destroying the existing block, populating search libraries, and reading the chip) should be refactored into a shared free function within a namespace to improve maintainability and ensure consistency.

References

Helper logic should be defined as a free function in a namespace rather than as a local lambda within a function.

gemini-code-assist · 2026-04-09T19:49:42Z

+      std::vector<odb::dbLib*> search_libs;
+      for (odb::dbLib* lib : db_->getLibs()) {
+        search_libs.push_back(lib);
+      }


The population of the search_libs vector can be simplified using std::vector::assign. This is more efficient and follows the general rule for copying elements from a dbSet to a std::vector.

std::vector<odb::dbLib*> search_libs; search_libs.assign(db_->getLibs().begin(), db_->getLibs().end());

References

To copy elements from a dbSet to a std::vector, use the vector::assign() method with iterators from the dbSet, as direct assignment is not supported.

github-actions · 2026-04-10T02:05:11Z

clang-tidy review says "All clean, LGTM! 👍"

osamahammad21

Remove writing a def file in the 3dbv for each chiplet and add a test case.

osamahammad21 · 2026-04-13T17:44:37Z

+
+  if (!chip_inst.external.def_file.empty()) {
+    if (chip->getBlock() != nullptr) {
+      odb::dbBlock::destroy(chip->getBlock());


you probably should log an error here. There can't be 2 instances of the same chiplet with a def file each.

osamahammad21 · 2026-04-13T17:46:07Z

+    odb::defin def_reader(db_, logger_, odb::defin::DEFAULT);
+    std::vector<odb::dbLib*> search_libs;
+    for (odb::dbLib* lib : db_->getLibs()) {
+      search_libs.push_back(lib);
+    }
+    def_reader.readChip(
+        search_libs, chip_inst.external.def_file.c_str(), chip, false);


This is the same code written in createChiplet, wrap in an inline function to reduce code duplication.

osamahammad21 · 2026-04-13T17:46:46Z

+    odb::dbBoolProperty::create(chip, "def_file_read", true);
+    if (auto* prop = odb::dbStringProperty::find(chip, "def_file_path")) {
+      prop->setValue(chiplet.external.def_file.c_str());
+    } else {
+      odb::dbStringProperty::create(
+          chip, "def_file_path", chiplet.external.def_file.c_str());
+    }


why? I don't think these properties are needed.

osamahammad21 · 2026-04-13T17:47:03Z

+    odb::dbBoolProperty::create(chip, "def_file_read", true);
+    if (auto* prop = odb::dbStringProperty::find(chip, "def_file_path")) {
+      prop->setValue(chip_inst.external.def_file.c_str());
+    } else {
+      odb::dbStringProperty::create(
+          chip, "def_file_path", chip_inst.external.def_file.c_str());
+    }


not needed.

osamahammad21 · 2026-04-13T17:49:27Z

+  auto* chip = inst->getMasterChip();
+  if (odb::dbProperty::find(chip, "def_file_read") != nullptr) {
+    YAML::Node external_node = instance_node["external"];
+    external_node["def_file"] = std::string(chip->getName()) + ".def";
+  }


This is going to end up in all instances of that chiplet having a def file defined. This is incorrect. There should only be one chipInst with a defined def_file. It doesn't matter which one, but it should be only one.

github-actions · 2026-04-14T22:17:54Z

clang-tidy review says "All clean, LGTM! 👍"

jorge-ferreira-pii · 2026-04-14T22:23:45Z

All requested changes have been made.

osamahammad21

The current implementation creates a block for every chiplet definition created. This is necessary for reading the bump map and populating the block with the bumps. We need, somehow, to delay reading the bump map to after we've read the defs of the master chiplet instances. It is gonna look ugly, but that's unavoidable with the current standard.

github-actions · 2026-04-16T19:50:00Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-06-15T19:57:31Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 21 days if no further activity occurs. Remove the Stale label or comment to keep it open.

osamahammad21

I'm not a fan of the call_depth_ approach. I understand the underlying reason for it: the bump map can only be created once the chiplet's block exists, so we need to guarantee the chiplet DEF is read before the bmap. The depth counter is how we currently force "all DEFs are in" before flushing the deferred bumps.

But this only holds when everything is reachable from a single top-level read (e.g. a read_3dbx that includes the .3dbv). What happens with a standalone read_3dbv followed by a separate read_3dbx? At the end of read_3dbv the depth is already back to 0, so the bumps get flushed, but the inst-level DEF doesn't arrive until the later read_3dbx command. The ordering guarantee breaks across separate commands, and no end-of-read flush can fix that.

I'd suggest a different direction: instead of deferring the bumps, create the block + bumps eagerly at .3dbv time and introduce a new defin mode dedicated to 3DBlox for the DEF read. It would:

operate on a block that already exists (created when the chiplet definition was read), rather than creating one from scratch and erroring if one is present;
handle duplicate data correctly: a bump can appear in both the bmap and the DEF, along with its associated bterm and net, so the DEF checks instead of duplicating or erroring.

This makes the DEF read order-independent and drops call_depth_ / pending_bmaps_ entirely.

Separately, we still need to track which chips have already had a DEF read and error out when 2+ instances each define a DEF for the same chiplet.

…0077) Per the 3DBlox standard (sec 4.3.1) the DEF file is associated with a ChipletInst (in the .3dbx) rather than the ChipletDef. Read it from either location (whichever exists) onto the master chiplet block. - Add defin::THREE_D_BLOX mode: reads a DEF onto an already-existing block, find-or-creating COMPONENTS/PINS/NETS and deduplicating data shared with the bump map (a bump may appear in both the bmap and the DEF) instead of erroring or duplicating. defin::readChip now reports success so the chiplet's DEF is marked read only on success. - 3dblox: create block+bumps eagerly at .3dbv time; read each chiplet's DEF once (chips_with_def_); error (ODB-547) when two instances of the same chiplet each carry a DEF. - Writers: emit def_file on a single ChipletInst in the .3dbx (no longer in the .3dbv), chosen deterministically by name; sort the 3dbv LEF_file output. - external spec: single-valued external fields (def_file/verilog_file/sdc_file) accept a scalar or a single-element list via BaseParser::extractSinglePathFromList and error (ODB-521) on more than one entry. Signed-off-by: Jorge Ferreira <jorge.ferreira@precisioninno.com>

openroad-ci assigned jorge-ferreira-pii Apr 9, 2026

github-actions Bot added the size/S label Apr 9, 2026

jorge-ferreira-pii requested a review from osamahammad21 April 9, 2026 19:47

gemini-code-assist Bot reviewed Apr 9, 2026

View reviewed changes

osamahammad21 requested changes Apr 13, 2026

View reviewed changes

jorge-ferreira-pii requested a review from osamahammad21 April 14, 2026 22:23

osamahammad21 requested changes Apr 16, 2026

View reviewed changes

github-actions Bot added size/M and removed size/S labels Apr 16, 2026

jorge-ferreira-pii requested a review from osamahammad21 April 16, 2026 19:50

github-actions Bot added the Stale A stale PR or issue subject to automated closure. label Jun 15, 2026

osamahammad21 requested changes Jun 17, 2026

View reviewed changes

openroad-ci force-pushed the fix-3DBlox-DEF-file branch from f15bf78 to 81b65a2 Compare June 18, 2026 02:43

openroad-ci requested a review from a team as a code owner June 18, 2026 02:43

openroad-ci requested a review from osamahammad21 June 18, 2026 02:43

openroad-ci force-pushed the fix-3DBlox-DEF-file branch from 81b65a2 to 99ead73 Compare June 18, 2026 03:07

github-actions Bot removed the Stale A stale PR or issue subject to automated closure. label Jun 18, 2026

openroad-ci force-pushed the fix-3DBlox-DEF-file branch from 99ead73 to a166700 Compare June 18, 2026 18:07

Uh oh!

Conversation

openroad-ci commented Apr 9, 2026

Uh oh!

github-actions Bot commented Apr 9, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 10, 2026

Uh oh!

osamahammad21 left a comment

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

osamahammad21 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 14, 2026

Uh oh!

jorge-ferreira-pii commented Apr 14, 2026

Uh oh!

osamahammad21 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 16, 2026

Uh oh!

github-actions Bot commented Jun 15, 2026

Uh oh!

osamahammad21 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants